-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove serde support #53
Conversation
The real kicker is this: serde-rs/serde#2120 (comment) The The only way to avoid a length prefix is to use |
Ya exactly. I notice you said in the linked thread you were switching to |
It was in the It's designed specifically for constant-time bytes handling. Using |
It looks like there is one project that is using this, here and here. I also realized, I don't think the p256 impl is DER-encoded. The serde impls appear to just be a thin wrapper around |
This PR removes serde support. This is following a conversation elsewhere about providing serde impls for byte slices. The argument for removing serde impls is:
[T; N]
serialization, or(T, T, ..., T)
serialization, then you miss out on optimizations for formats with byteslice representations. This has a horrible slowdown, which I've experience withwasm_bindgen
. This is whyserde_bytes
exists, and it cannot be merged into upstream (see above conversation)The current p256I don't think this is true. It's the bytes given byserde::Serialize
implementation is the DER encoding of the public key. This is not something I noticed, and certainly doesn't match the raw byte representation of the other public key serializations. I should not have autoderived without looking.to_bytes()
.In short, people have different needs when it comes to serializing HPKE values with
serde
, and any choice necessarily excludes other options. So rather than making that choice for the user, this crate should instead just expose its normalSerializable
trait (with methodsto_bytes
andwrite_exact
), and let the user implement what they need.Looking for feedback on this, since it's removing a feature that might be in use currently.
cc @marmeladema @bwesterb @tarcieri